Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

annotate alloc on unixes. #548

Merged
merged 2 commits into from
Sep 25, 2022
Merged

Conversation

devnexen
Copy link
Collaborator

@devnexen devnexen commented Aug 5, 2022

No description provided.

src/snmalloc/mem/localalloc.h Outdated Show resolved Hide resolved
#if defined(_MSC_VER)
# define ALLOCATOR __declspec(allocator) __declspec(restrict)
#elif defined(__GNUC__)
# define ALLOCATOR __attribute__((__malloc__))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be

Suggested change
# define ALLOCATOR __attribute__((__malloc__))
# define ALLOCATOR __attribute__((malloc))

and probably we want to do something like

Suggested change
# define ALLOCATOR __attribute__((__malloc__))
# define ALLOCATOR __attribute__((malloc, malloc(dealloc)))

to associate the deallocator, but perhaps that means it has to be

#  define ALLOCATOR(dealloc) __attribute__((malloc, malloc(dealloc)))

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

define ALLOCATOR(dealloc) attribute((malloc, malloc(dealloc)))

I thought of that but clang does not support this form.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This form is somewhat useful for static analysis but it likely to lead to miscompiles as currently proposed to be integrated in LLVM. The problem is that the compiler doesn't track the hierarchy of allocators that are in use. An allocation path might have a call to alloc, whereas the deallocation path might end up being inlined to the point where it knows to call munmap or similar. I think that isn't a problem for the most recent snmalloc (we never hand actually unmap memory) but it might be a problem if we add the same attributes for the ranges (if you do a large thread-local allocation and free then we'll still hit the allocator path but may push it back into the thread's large chunk cache on deallocation and so enough might be inlined to confuse the optimiser into thinking that you've allocated with one allocator and freed with another).

Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, @nwf-msr are you happy with the updates?

@nwf-msr
Copy link
Contributor

nwf-msr commented Aug 11, 2022

LGTM, @nwf-msr are you happy with the updates?

LGTM

@devnexen
Copy link
Collaborator Author

ping :)

@mjp41 mjp41 merged commit a060462 into microsoft:main Sep 25, 2022
@mjp41
Copy link
Member

mjp41 commented Sep 25, 2022

ping :)

Sorry, I forgot about this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants